Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

only invoke didChange if a node is still present. #11558

Conversation

stefanpenner
Copy link
Member

noticed this in : #11513
introduced: https://github.com/emberjs/ember.js/pull/11536/files

[fixes #11564]

  • add test
  • add similar guard for willChange

@stefanpenner stefanpenner force-pushed the only-invoke-still-present-chain-entries branch from 3698b56 to 77c6dfe Compare June 26, 2015 05:50
@rwjblue
Copy link
Member

rwjblue commented Jun 26, 2015

LGTM, I'd like to land the RC/AC PR soonish, timeline on adding test (then rebasing that PR)?

@stefanpenner
Copy link
Member Author

I'll be adding an explicit test after breakfast today.

Have you had a chance to test out AC/RC in your own apps?

@rwjblue
Copy link
Member

rwjblue commented Jun 26, 2015

@stefanpenner - Yes, I have tested in a couple apps (I'm not using all possible macros though) and things worked (all tests pass). The only way to get more testing, is to land in canary and/or beta...

@stefanpenner
Copy link
Member Author

The only way to get more testing, is to land in canary and/or beta...

Ya, im totally fine with it getting pulled in. :)

@rwjblue
Copy link
Member

rwjblue commented Jun 26, 2015

closed by #11513

@rwjblue rwjblue closed this Jun 26, 2015
@rwjblue rwjblue deleted the only-invoke-still-present-chain-entries branch June 26, 2015 23:49
@stefanpenner stefanpenner restored the only-invoke-still-present-chain-entries branch June 27, 2015 23:54
@stefanpenner stefanpenner reopened this Jun 27, 2015
@rwjblue
Copy link
Member

rwjblue commented Jun 28, 2015

This change already exists in master, and I believe that a rebase will make this commit a noop.

@stefanpenner
Copy link
Member Author

This change already exists in master, and I believe that a rebase will make this commit a noop.

needs a test still

@rwjblue
Copy link
Member

rwjblue commented Jun 28, 2015

Yep, completely agreed.

@stefanpenner
Copy link
Member Author

if its closed i'll forget to write the test

@stefanpenner stefanpenner force-pushed the only-invoke-still-present-chain-entries branch from 77c6dfe to 20b16a5 Compare June 29, 2015 19:53
stefanpenner added a commit that referenced this pull request Jun 30, 2015
…-chain-entries

only invoke didChange if a node is still present.
@stefanpenner stefanpenner merged commit 0c8910d into emberjs:master Jun 30, 2015
@stefanpenner
Copy link
Member Author

@rwjblue friendly reminder to pull this into beta

@stefanpenner stefanpenner deleted the only-invoke-still-present-chain-entries branch June 30, 2015 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chains: Uncaught TypeError: Cannot read property 'willChange' of undefined
2 participants